PR Review

Summary

This PR fixes a real concurrency bug in gnoi-shutdown-daemon. When multiple DPUs shut down in parallel, worker threads shared a single Redis DBConnector socket. Concurrent hget/Table.get calls interleaved on the wire, causing replies to be matched to the wrong requests — producing crossed IP/port values like 169.254.200.1:169.254.200.2.

The fix gives each per-DPU worker thread its own CONFIG_DB and STATE_DB connection and threads them through _handle_transition() and _wait_for_gnoi_halt_in_progress().

Root Cause Analysis — Correct

The diagnosis is sound. swsscommon DBConnector is a single non-thread-safe Redis socket. The failure mode (DPU0's IP with DPU1's value in the port slot) is consistent with crossed read replies on a shared connection.

The threaded shutdown path in handle_and_cleanup() previously called _handle_transition() using self._config_db and self._db created once in main().

Code Review

What looks good:

1. Targeted fix — Only the DB reads inside the threaded path are changed. The main pubsub loop still uses the shared config_db safely (single-threaded).
2. Backward-compatible API — Optional config_db/state_db parameters with fallback to self._config_db/self._db preserve existing unit-test call sites.
3. All threaded DB access is covered:
   • _wait_for_gnoi_halt_in_progress() → state_db
   • get_dpu_ip() / get_dpu_gnmi_port() → config_db
   • _clear_halt_flag() uses chassis platform API, not Redis — correctly left alone
4. Clear inline comment in handle_and_cleanup() explaining why per-thread connections are needed.
5. Low risk — Small, focused diff with a clear causal link to Redmine #5136310.

Minor suggestions (non-blocking):

1. Add a unit test for the per-thread path — Existing tests call _handle_transition() directly (fallback path) or mock threading.Thread in test_main_loop_flow, so the new db_connect calls in handle_and_cleanup are never exercised. A small test that invokes handle_and_cleanup directly and asserts db_connect("CONFIG_DB") / db_connect("STATE_DB") are passed into _handle_transition would lock in the fix and prevent regressions.

2. Consider logging the gNOI target — The crossed-read bug produced target=169.254.200.1:169.254.200.2. Logging dpu_ip:port in _send_reboot_command before the RPC would make future misconfiguration easier to spot. Optional, not required for this fix.

3. Connection lifecycle — Per-thread connections are not explicitly closed. For short-lived shutdown threads this is fine and matches common SONiC daemon patterns. No action needed unless you see connection buildup under repeated shutdown cycles.

Testing Assessment

Process / Compliance

• DCO sign-off missing — Commit f68f548 has no Signed-off-by line. Other commits in this repo include it. Please add before merge:

  Signed-off-by: Gagan Ellath <gpunathilell@nvidia.com>
  

Verdict

Approve with minor nits.

The root cause is correctly identified, the fix is the right approach for parallel DPU shutdown, and the change is minimal and low-risk. I'd merge after:

1. Adding the DCO Signed-off-by line
2. (Recommended) Adding a unit test that verifies per-thread DB connections are created and passed through in handle_and_cleanup

The crossed-read failure (169.254.200.1:169.254.200.2 → lookup tcp/169.254.200.2: unknown port) should be resolved by this change. Cherry-pick to 202605_RC as noted in the PR description is appropriate given where the bug was seen.